Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert internal rc_getline() calls to getline() calls #278

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

williamh
Copy link
Contributor

getline has been in posix since POSIX.1-2008, so it should be safe for
us to use it instead of using our wrapper function.

williamh added a commit to williamh/openrc that referenced this pull request Dec 24, 2018
getline has been in posix since POSIX.1-2008, so it should be safe for
us to use it instead of using our wrapper function.

This fixes OpenRC#278.
@williamh
Copy link
Contributor Author

I would like to call for some testers to make sure this works before I merge it. Please report back here.

@dwfreed
Copy link
Contributor

dwfreed commented Dec 24, 2018

getline's behavior does not match rc_getline's in various ways, like whether the newline is included if found (getline: yes, rc_getline: no). You should just make rc_getline a wrapper for getline, so that this can be fixed in one location, instead of having to do it in several.

@williamh
Copy link
Contributor Author

@dwfreed The whole reason rc_getline was written was because getline wasn't standard when it was needed. Also, the code already uses both rc_getline and getline. I think I would still rather migrate to getline.

@williamh
Copy link
Contributor Author

@dwfreed I see that rc_getline is only used in librc. If I do a wrapper, I would want it to be private to the library, not available for general use.

@vapier
Copy link
Member

vapier commented Jan 4, 2019

the low usage rates implies it's not a big deal to drop even if it slightly simplifies trailing newline semantics. everyone is used to the posix api at this point so it's a bit of a lost battle.

technically we'd need to keep the symbol just to not break abi.

@williamh
Copy link
Contributor Author

@vapier I could use some insight here. Is it ok in this case because of the low usage to break abi and restore it if someone complains? If not, I guess it means that if I keep the symbol I have to keep the function?

williamh added a commit to williamh/openrc that referenced this pull request Jan 27, 2022
getline has been in posix since POSIX.1-2008, so it should be safe for
us to use it instead of using our wrapper function.

This fixes OpenRC#278.
@dwfreed
Copy link
Contributor

dwfreed commented Jan 27, 2022

I'm not able to do a proper review right now, but a quick look indicates many of the consumers of rc_getline expect and need the newline dropping semantics. The sheer quantity suggests it would be better to just use rc_getline as a wrapper around getline to drop the newline, otherwise you will have to implement that in several places, which seems error prone and a waste of effort.

williamh added a commit to williamh/openrc that referenced this pull request Jan 16, 2023
getline has been in posix since POSIX.1-2008, so it should be safe for
us to use it instead of using our wrapper function.

This fixes OpenRC#278.
    getline has been in posix since POSIX.1-2008, so it should be safe for
    us to use it instead of our wrapper function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants